feat(auto-salvage): v2 support #675
Conversation
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
4 times, most recently
from
1f4b165
to
5eacb5c
Compare
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
5eacb5c
to
149868d
Compare
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
149868d
to
479e276
Compare
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
479e276
to
227ad47
Compare
WalkthroughThe changes introduce a new boolean field, Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
pkg/proxy/volume.go (1)
74-77: Document the purpose of the boolean parameter in EngineGet.The addition of
falseparameter toc.EngineGet()needs documentation to explain its purpose and impact, especially since it appears to be related to the auto salvage feature.Consider adding a comment explaining what this boolean parameter controls:
+ // The boolean parameter controls whether to include salvage-related information recv, err := c.EngineGet(req.EngineName, false)pkg/proxy/replica.go (2)
154-155: Document the purpose of the boolean parameter in EngineGet.The
falseparameter's purpose is not immediately clear from the context. Please add a comment explaining what this boolean parameter controls and why it's set tofalsehere.
231-232: Maintain consistent error handling patterns.The error handling here differs from the
ReplicaListmethod. Consider usingerrors.Wrapffor consistent error handling:-return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to get engine %v: %v", req.EngineName, err) +return nil, grpcstatus.Error(grpccodes.Internal, errors.Wrapf(err, "failed to get engine %v", req.EngineName).Error())pkg/proxy/snapshot.go (4)
Line range hint
158-165: Copy snapshot labels into the response.Currently, the
Labelsfield inresp.Disksis initialized as an empty map. If thesnapshotobjects contain labels, consider copying them to preserve important metadata.Apply this diff to include the labels:
resp.Disks[snapshotName] = &rpc.EngineSnapshotDiskInfo{ Name: snapshot.Name, Parent: snapshot.Parent, Children: snapshot.Children, Removed: false, UserCreated: snapshot.UserCreated, Created: snapshotTime, Size: strconv.FormatUint(snapshot.ActualSize, 10), - Labels: map[string]string{}, + Labels: snapshot.Labels, }
Line range hint
271-273: Correct the error message inSnapshotRevert.In the error handling for
c.EngineSnapshotRevert, the error message states "failed to create snapshot" instead of "failed to revert snapshot". Please correct the error message for accurate logging.Apply this diff to fix the error message:
if err != nil { - return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to create snapshot %v: %v", req.Name, err) + return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to revert snapshot %v: %v", req.Name, err) }
Line range hint
276-279: Implement theSnapshotClonemethod for V2 engines or handle unimplemented methods appropriately.The
SnapshotClonemethod inV2DataEngineProxyOpsis currently unimplemented, returning an unimplemented error. This may lead to issues if the method is invoked for V2 engines. Consider implementing this functionality or providing proper error handling to prevent unexpected behavior.Would you like assistance in implementing the
SnapshotClonemethod for V2 engines? I can help draft the necessary code changes or open a GitHub issue to track this task.
Line range hint
315-318: ImplementSnapshotHashandSnapshotHashStatusmethods for V2 engines.The methods
SnapshotHashandSnapshotHashStatusinV2DataEngineProxyOpsare currently unimplemented, returning unimplemented errors. To fully support the auto-salvage feature for V2 volumes, consider implementing these methods or ensuring they are appropriately handled to prevent runtime errors.Would you like assistance in implementing these methods? I can help draft the code or open a GitHub issue to track this enhancement.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (13)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/go-common-libs/utils/misc.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/client/client.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/replica.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/server.gois excluded by!vendor/**vendor/github.com/longhorn/types/pkg/generated/imrpc/instance.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/golang.org/x/exp/LICENSEis excluded by!vendor/**vendor/golang.org/x/exp/PATENTSis excluded by!vendor/**vendor/golang.org/x/exp/constraints/constraints.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (5)
- pkg/client/instance.go (2 hunks)
- pkg/instance/instance.go (2 hunks)
- pkg/proxy/replica.go (2 hunks)
- pkg/proxy/snapshot.go (1 hunks)
- pkg/proxy/volume.go (1 hunks)
🔇 Additional comments (6)
pkg/proxy/volume.go (1)
Line range hint
141-143: Prioritize V2 method implementations based on auto salvage requirements.Multiple V2 methods are marked as TODO. Given that this PR aims to support auto salvage for v2 volumes, it would be helpful to:
- Identify which methods are critical for the auto salvage feature
- Prioritize their implementation
- Document any dependencies between these methods
Let's check which methods might be required for auto salvage:
Also applies to: 182-184, 224-226, 266-268, 308-310, 350-352
pkg/client/instance.go (2)
94-94: LGTM: Clear and well-named field additionThe new
SalvageRequestedboolean field is appropriately named and aligns with the PR objective to support auto salvage functionality for v2 volumes.
151-151: Consider adding documentation for the salvage request functionalityWhile the implementation is correct, it would be beneficial to add documentation about:
- When the
SalvageRequestedflag should be set- Any prerequisites or conditions that should be met before requesting salvage
- Potential impacts of enabling salvage mode
Additionally, let's verify the integration with the SPDK layer:
pkg/proxy/snapshot.go (1)
138-138: Confirm the use of the boolean parameter inEngineGet.In
engine, err := c.EngineGet(req.EngineName, true), the second parametertruealters the behavior ofEngineGet. Please ensure that this change is intentional and that it aligns with the function's expected parameters. Verify that passingtrueis appropriate in this context.pkg/instance/instance.go (2)
154-154: Addition of 'SalvageRequested' parameter to EngineCreateThe inclusion of
req.Spec.SpdkInstanceSpec.SalvageRequestedin theEngineCreatecall enables the engine to handle salvage requests for v2 data engines, supporting the auto salvage feature as intended.
273-273: Verify the use of 'false' in EngineGet and ReplicaGet callsThe addition of
falseas the second argument inc.EngineGet(req.Name, false)andc.ReplicaGet(req.Name, false)alters the behavior of instance retrieval. Please ensure that passingfalseis intentional and aligns with the expected functionality of these methods.To confirm that
falseis the correct value, you can review the method signatures and documentation ofEngineGetandReplicaGetto ensure they behave as expected when this parameter isfalse.Also applies to: 279-279
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
227ad47
to
f2ab2bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/client/instance.go (1)
94-94: Add documentation for the new SalvageRequested field.Please add a comment explaining the purpose and usage of this field to improve code maintainability.
type EngineCreateRequest struct { ReplicaAddressMap map[string]string Frontend string InitiatorAddress string TargetAddress string UpgradeRequired bool + // SalvageRequested indicates whether a salvage operation should be performed during engine creation SalvageRequested bool }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (13)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/go-common-libs/utils/misc.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/client/client.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/replica.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/server.gois excluded by!vendor/**vendor/github.com/longhorn/types/pkg/generated/imrpc/instance.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/golang.org/x/exp/LICENSEis excluded by!vendor/**vendor/golang.org/x/exp/PATENTSis excluded by!vendor/**vendor/golang.org/x/exp/constraints/constraints.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
- pkg/client/instance.go (2 hunks)
- pkg/instance/instance.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/instance/instance.go
🔇 Additional comments (2)
pkg/client/instance.go (2)
Line range hint
94-151: Implementation looks good overall.The changes appropriately implement the foundation for v2 volume auto-salvage support. The new field is well-integrated into the existing flow and maintains backward compatibility.
151-151: Verify RPC layer integration for SalvageRequested.The field is correctly propagated to the SPDK instance spec. Let's verify that the RPC layer is prepared to handle this new field.
✅ Verification successful
Let me gather more information about the RPC implementation to verify the integration.
RPC layer integration for SalvageRequested is properly implemented
The field is correctly integrated across all required layers:
- Defined in the protobuf spec (
instance.pb.goandspdk.pb.go)- Properly propagated through the SPDK engine client and server implementations
- Correctly structured in both
EngineCreateRequestandSpdkInstanceSpec🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the RPC proto files have been updated to include the SalvageRequested field # Search for SalvageRequested field definition in proto files echo "Checking proto files for SalvageRequested field:" rg -l "SalvageRequested" --type proto # Check for any related RPC handler implementations echo -e "\nChecking for RPC handler implementations:" rg "SalvageRequested" --type go -g "**/*server*.go"Length of output: 560
Script:
#!/bin/bash # Check proto definitions in vendor directory echo "Checking proto definitions in vendor directory:" rg -l "SalvageRequested" --type=proto ./vendor # Check the actual implementation in SPDK engine echo -e "\nChecking SPDK engine implementation:" rg "SalvageRequested" -A 5 ./vendor/github.com/longhorn/longhorn-spdk-engine/ # Check for any related struct definitions echo -e "\nChecking related struct definitions:" ast-grep --pattern 'type $_ struct { $$$ SalvageRequested $_ $$$ }'Length of output: 6920
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
4 times, most recently
from
2576ccf
to
48cfc80
Compare
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
2 times, most recently
from
f40b8f8
to
57e8bbb
Compare
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
2 times, most recently
from
1d79722
to
b3f1700
Compare
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
b3f1700
to
f324a73
Compare
Which issue(s) this PR fixes:
Issue longhorn/longhorn#8430
What this PR does / why we need it:
Include snapshot disk info in the proxy.ReplicaList response.
Special notes for your reviewer:
NoneAdditional documentation or context
None